-
-
Notifications
You must be signed in to change notification settings - Fork 20
OAuth support #1314
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
OAuth support #1314
Conversation
|
Nice work! This seems like all great improvements over my first step. I don't have any faith at this point that @frenck even looks at this repo, but we can dream |
I do. |
|
Would it be possible to split some of this up into individual PRs? It will make reviewing easier and could speed up acceptance of the PR. |
|
Not really. Only thing I can think of is the token storage abstract class with 2 abstract functions, plus a few lines of code calling it, understanding that takes as much time as reading this long sentence. All the other stuff work together to make the OAuth functionality to work at all. If I remove anything (into a separate PR), the remaining is broken, so doesn't makes sense in my opinion. |
Fair enough. I did skim the code. But was mostly basing my statement on "Plus fixed some things" in your PR header. Carry on. |
|
I think my old PR #1260 would be the other splitting up, but that one isn't as functional as this, and wasn't reviewed either |
Proposed Changes
Continuation of / supersedes #1260.
Plus fixed some things:
Plus added possibility to automatically store and retrieve the received OAuth token.
Related Issues
closes #1260 #1140